-
Notifications
You must be signed in to change notification settings - Fork 438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
developed dao logic #427
base: main
Are you sure you want to change the base?
developed dao logic #427
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several critical issues that need to be addressed, such as incorrect table names in SQL statements and improper handling of Optional
in the findById
method. Additionally, storing database credentials directly in the code poses a security risk. Please address these issues to ensure the functionality and security of the application. Keep up the good work, and once these changes are made, your code will be in great shape! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
public static void main(String[] args) { | ||
private static final Injector injector = Injector | ||
.getInstance("mate.academy.lib"); | ||
private static final BookDao dao = (BookDao) injector.getInstance(BookDao.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BookDao
interface specifies that the findById
method should return an Optional<Book>
, but the current code calls dao.findById(1L)
without handling the Optional
. Consider using Optional
methods like ifPresent
, orElse
, or orElseThrow
to handle the result properly.
public class BookDaoImpl implements BookDao { | ||
|
||
@Override | ||
public Book create(Book book) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the create
method, ensure that the SQL statement uses the correct table name books
as specified in the task description. The current statement uses book
, which may lead to errors if the table name is incorrect.
} | ||
|
||
@Override | ||
public Optional<Book> findById(Long id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the findById
method, ensure that the SQL statement uses the correct table name books
as specified in the task description. The current statement uses book
, which may lead to errors if the table name is incorrect.
|
||
@Override | ||
public List<Book> findAll() { | ||
String sqlFindAllStatement = "SELECT * FROM book"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the findAll
method, ensure that the SQL statement uses the correct table name books
as specified in the task description. The current statement uses book
, which may lead to errors if the table name is incorrect.
} | ||
|
||
@Override | ||
public Book update(Book book) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the update
method, ensure that the SQL statement uses the correct table name books
as specified in the task description. The current statement uses book
, which may lead to errors if the table name is incorrect.
@Override | ||
public boolean deleteById(Long id) { | ||
int idPlace = 1; | ||
String sqlDeleteStatement = "DELETE FROM book WHERE id = ? "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the deleteById
method, ensure that the SQL statement uses the correct table name books
as specified in the task description. The current statement uses book
, which may lead to errors if the table name is incorrect.
import mate.academy.exception.DataProcessingException; | ||
|
||
public class ConnectionUtils { | ||
private static final String DB_URL = "jdbc:mysql://localhost:3306/test"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the database URL in DB_URL
is correct and matches the schema you have set up for this task. The current URL points to jdbc:mysql://localhost:3306/test
, which may not be the intended schema for your application.
DB_PROPERTIES.put("user", "root"); | ||
DB_PROPERTIES.put("password", "4321"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing database credentials directly in the code is not a secure practice. Consider externalizing these properties to a configuration file or environment variables to enhance security.
import java.util.Objects; | ||
|
||
public class Book { | ||
private long id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The id
field should be of type Long
instead of long
to match the task description and to allow for null values when the ID is not yet assigned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on implementing the core functionalities! 🎉 While the code is generally well-structured and follows good practices, there are a few areas for improvement. Pay attention to the comparison of Long objects in the equals method, enhance security by managing database credentials more securely, and consider logging connection attempts for better traceability. Also, ensure consistency in error handling by using specific exceptions. Keep up the good work and use these suggestions to refine your skills further!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
statement.setLong(idPlace,book.getId()); | ||
statement.setString(titlePlace,book.getTitle()); | ||
statement.setBigDecimal(pricePlace,book.getPrice()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider setting the parameters in the order they appear in the SQL statement to improve readability. This means setting titlePlace and pricePlace before idPlace, as they appear first in the SQL statement.
if (affectedRows > 0) { | ||
return book; | ||
} else { | ||
throw new RuntimeException("Failed to update book with id: " + book.getId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of throwing a generic RuntimeException, consider using a more specific exception type or the existing DataProcessingException to maintain consistency in error handling.
private static final String PASSWORD = "4321"; | ||
private static final String USER = "root"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing database credentials directly in the code is not secure. Consider using environment variables or a secure vault to manage sensitive information like database passwords.
} | ||
|
||
public static Connection getConnection() throws SQLException { | ||
return DriverManager.getConnection(DB_URL,DB_PROPERTIES); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good practice to log the connection attempts and any exceptions that occur during the connection process for better traceability and debugging.
return false; | ||
} | ||
Book book = (Book) o; | ||
return id == book.id && Objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When comparing Long objects, it's safer to use Objects.equals(id, book.id) instead of id == book.id to avoid potential issues with object references and null values.
No description provided.